Test app #1687
Test app #1687
Conversation
|
The whole test suite is currently being run synchronously on the main thread. This hasn't caused any problems so far, but I'm sure it will soon, so that's the next thing I'll look at. We talked about the possibility of making Toga itself thread-safe, so that every method in the Toga API would run itself on the main thread if necessary. But if we did that, and then ran the tests on a background thread, it wouldn’t be representative of the most common case of a single-threaded app, because we'd be executing each Toga API call on the main thread separately, with the possibility of other things happening in between. So I think the best approach is to run the pytest main loop on a background thread, but arrange for each test function and its supporting fixtures to be run on the main thread. The pytest thread would block until the function completed, and any exception would be re-raised on the pytest thread so it could be reported. Fortunately It looks like pytest has a comprehensive hook system which should make this fairly straightforward. And however we end up dealing with this, we can expose it as a pytest plugin so that other apps using Toga can do the same thing. |
There was a problem hiding this comment.
@freakboy3742: This is still incomplete, but I'd be interested to know your thoughts on the general approach.
There was a problem hiding this comment.
tl;dr - I like what I'm seeing. Some of the rough edges are definitely due to not being in "briefcase run --test" format yet, plus some of the complications you've flagged about shared sources. I've got a couple of questions about the usage of fixtures, plus a couple of cosmetic quirks, but on the whole, this definitely looks like it's heading in the right direction.
A couple of "next steps" features I'd like to see:
- Geometry tests. Proving that a widget expands to the expected size in a given layout
- Tests of a widget like NumericalInput where there's a range of inputs, and the input isn't necessarily the same as the output
- Tests of a widget that has focus-driven behavior (e.g., text input doing input validation)
- A "wait for the GUI to do it's thing" await call; you've flagged the possible need for one of these, but it strikes me it could be a bit of a "draw the rest of the owl" situation
|
|
||
|
|
||
| @fixture | ||
| async def new_widget(): |
There was a problem hiding this comment.
It took me a while to work out what was going on here; 2 questions coming from this:
- Do all fixtures have to be async?
- Is a fixture the right model here - especially for the "broader layout"? Might it not be easier to have a
simple_layout(widget)which puts the test widget into a simple box layout?
There was a problem hiding this comment.
Do all fixtures have to be async?
Yes, if they call any Toga APIs, because making them async is what causes them to run on the main thread.
Is a fixture the right model here - especially for the "broader layout"? Might it not be easier to have a
simple_layout(widget)which puts the test widget into a simple box layout?
I'm not quite sure what you mean, but I'll adjust the fixture names and dependencies so it's clearer what each one is actually doing.
|
I've now added an initial set of tests for Label, Button and Slider, with probe implementations for Android and Windows. Running on Android will require beeware/briefcase-android-gradle-template#58. This is not intended to give complete coverage of any widget: that can be done in later PRs. In particular:
|
| class SimpleProbe: | ||
| def __init__(self, main_box, widget): | ||
| native_box = main_box._impl.native | ||
| assert native_box.getChildCount() == 1 |
There was a problem hiding this comment.
It's not clear to me why this is a feature of the probe, rather than an assertion related to the layout. This structure seems to be strongly bound to a specific layout; when we start adding more complex tests of layout, we're going to need entirely separate probe classes to support each possible layout scheme.
Would it make more sense to make Probe be purely the "probing" bits; write a probe for Box to abstract the "get my list of children"-type behavior, and add an assertion that uses the box probe and the widget probe in combination to verify impl relationships?
There was a problem hiding this comment.
Good point: I've reduced the Probe constructors to just be "find this widget inside this container", and any more specific layout assertions can wait until we write the layout tests.
There was a problem hiding this comment.
And I've separated out the function that instantiates the probe so it can be called from more complex layouts.
test/tests/widgets/test_button.py
Outdated
| return toga.Button("") | ||
|
|
||
|
|
||
| async def test_press(widget, probe): |
There was a problem hiding this comment.
This is one of the parts of pytest that I'm not a huge fan of. I'm willing to be convinced otherwise, but the "implied arguments" behavior of fixtures is one of those things that seems really clever, but contrary to understanding what a test is actually doing. It requires the mental model of the reader to match the mental model of the author, and the mental model of what pytest is actually doing.
To my mind, an explicit construction of a probe over a widget (with an explicit import of that probe class) would make more sense; and if a sequence of tests is going to use the same probe construction a lot, then it should be pulled out as a fixture of that probe.
There was a problem hiding this comment.
OK, let's discuss this tomorrow.
There was a problem hiding this comment.
After discussion, we came to the conclusion that:
- Yes, this is potentially confusing for people not particularly familiar with Pytest; however, it's also very much learnable knowledge, so anyone with moderate experience with Pytest should be familiar with the trick being used here
- In the specific case of these tests, we have a need for "a widget and a probe" for every test. That's likely to be a common property of almost all the individual widget tests - enough that having a single simple abstraction makes sense.
- If we have other multi-widget or complex-layout requirements, we can put them in separate testing packages with isolated conftest modules that won't inherit this specific widget + probe definitions.
There was a problem hiding this comment.
I think for someone who understands the basics of pytest, the only unusual thing going on here is that we have a probe fixture in a conftest file depending on a widget fixture in a test module. This is less common than the other direction, but conceptually it's no different from a method in a base class calling an abstract method which must be implemented by a subclass.
So maybe it would be clearer if the conftest file actually contained an abstract widget fixture which raises NotImplementedError, with a comment explaining how the test modules are supposed to override it. That way you could read the conftest file in isolation without being confused about where the widget was coming from. And if you forgot to define widget in a test module, then instead of pytest's generic "missing fixture" error, you would get a NotImplementedError leading you directly to the relevant information.
Anyway, I'll revisit this along with the comment above about the probe constructor.
Add a CI configuration for the testbed app
That all sounds reasonable: fixed by merging mhsmith#1. |
|
I've had enough of these random Codecov failures, and we're not really using it, so I've just removed it. |
|
Feel free to push directly to this branch if you know what's happening with the Linux CI failure. |
|
I guess to prevent Codecov leaving comments it would need to be disabled in the repository settings as well. |
Bother. Linux is building inside Docker, so the local file reference "../" doesn't resolve when running inside the Docker container. The "absolute path" approach used by Chaquopy won't work either. I've opened beeware/briefcase#992 to track this; I'm working on a patch. |
|
I've uninstalled CodeCov from the project; agreed that it has outstayed its welcome. |
|
The fix for beeware/briefcase#992 works great, so the Linux app is building in CI now, but when it tries to run it simply segfaults. Yet it works fine on my Linux machine. The only thing I can think of is maybe we should be running it through xvfb-run, as the backend unit tests do in tox.ini. Though if it was that, I would expect it to give a more useful error message. |
Well, it turns out it was that. And I can reproduce the crash on my own Linux machine simply by setting the DISPLAY environment variable to empty to prevent the app from contacting the X server. That's a pretty terrible user experience, but I don't know which component is to blame. |
|
Remember when the support testbed app was reporting tests taking negative time? Well, if you look at this line in today's log, it appears that the emulator's clock went backwards by two and a half minutes. My guess is that it started up with an inaccurate time, and then corrected itself from the internet. |
Annoying, but at least it works once you know what the problem is, and it's something we can document as part of a CI testing guide for Briefcase. |
At this stage I haven't yet set the project up to use separate tests per platform. I expect this would be done in pyproject.toml by making the
windowssection contain the linesources = ["../winforms/tests"], and so on.So far, I've added example Winforms tests for:
PR Checklist: